Copilot wasn't able to review this pull request because it exceeds the maximum number of files (300). Try reducing the number of changed files and requesting a review from Copilot again.

Written by Claude (Opus 5) with minimal oversight from Theodor. This review was produced by the compound-engineering ce-code-review skill: ten reviewer agents over the full diff, merged and deduplicated, then put through an independent validation pass (4 of 15 findings were dropped there — see Coverage). Treat the findings as a starting point, not a verdict from a human reviewer. Line references are against head 445e6999.

Code Review -- PR #548 "Feat/auth image collections [OWTR #1]"

Scope: pr:548 (vevcom/projectNext), base main, head feat/auth-image-collections @ 445e6999, merge-base 854dbe05. 402 files, +8299 / -5533, 82 commits. Scope mode: PR-remote (local tree is on an unrelated branch; all inspection ran against the fetched PR head). No untracked files excluded.

Intent (from the PR body, explicit): split the flat images service into a sub-service plus per-domain implementations, put every dynamic collection behind a two-level visibility model (see vs administrate), and move standard images from seeded rows to config-declared files -- without weakening authorization, losing stored files, or breaking existing image consumers across the CmsImage -> Image move.

Reviewer team: correctness, project-standards (repo-root CLAUDE.md governs every changed path), security (new authorizer + per-level checks), adversarial (auth + persistence writes + file deletion), testing (two new suites, large behavior change), maintainability (13k executable changed lines, new abstractions), data-migration (Prisma schema retargets Flair.image/Committee.logoImage, SpecialCmsImage shrinks), reliability (store deletion, seed races), performance (paging + visibility filters), previous-comments (PR already has a review and two comments).

Triage Groups

P0 -- Critical

#4 -- omegaOrderOperations.readCurrent({}) called without bypassAuth at four sites -- src/services/users/operations.ts:38 (also groups/committees/operations.ts:261, groups/interestGroups/operations.ts:16, omegaOrder/operations.ts:20)

• Why it matters: Creating a user (invitation flow), creating a committee, creating an interest group, and incrementing the omega order now throw UNAUTHORIZED for any session that holds the operation's own permission but not OMEGA_ORDER_READ -- an admin permission, not a default or membership one. All four previously called the unauthenticated helper readCurrentOmegaOrder(), deleted in this PR.
• Fix (mechanical): add bypassAuth: true to each of the four calls, matching omegaOrder/operations.ts:64 in this same diff.
• Confidence: 100, validator-confirmed (bypassAuth resolves to false for nested calls at serviceOperation.ts:250). Single reviewer (project-standards).
• Note the irony: the PR body describes fixing exactly this bug for visibilityOperations.update, while introducing it at four more sites.

#5 -- Visibility update deletes all requirements outside a transaction, so a failed save opens the collection to everyone -- src/services/visibility/operations.ts:23

• Why it matters: visibilityOperations.update runs visibilityRequirement.deleteMany and then a separate visibility.update re-create, with no transaction and no opensTransaction. Any failure in between -- a non-existent groupId/order (both real FKs), or two conditions colliding on @@unique([visibilityRequirementId, groupId, order]) -- leaves zero requirements, and checkVisibility treats an empty list as "everyone". It is reachable directly through updateDynamicImageCollectionRegularLevel/AdminLevelVisibilityAction.
• Fix (mechanical): set opensTransaction: true and wrap both statements in one prisma.$transaction, mirroring imageOperations.destroyCollection. Optionally de-duplicate conditions by (groupId, order) before the write so the normal UI path never trips the unique index.
• Confidence: 75, validator-confirmed. Corroborated by two independent reviewers (security, adversarial).

#7 -- Special-collection panel authorizer re-runs on internal calls, blocking self-service uploads -- src/services/images/subservice/special/implement.ts:84

• Why it matters: A normal member cannot change their own profile picture. The settings page renders the uploader (userAuth.updateProfileImage passes on username match), but the server call fails: uploadImage internally calls readCollection({}), whose authorizer is profileImagesImagePanelAuth = RequirePermission('USERS_UPDATE'). internalCall neuters only the sub-operation's own authorizer, not the nested one. The same mismatch hits committeeOperations.updateLogo, committeeOperations.create, and ombulOperations.create.
• Fix (mechanical): give implementSpecialCollection an internal reader (or call readCollection({ bypassAuth: true })) for uploadImage, destroyImage, and readPageOfImagesInCollection; keep the panel authorizer only on the externally exposed specialCollectionPanelOperations.readCollection.
• Confidence: 75, validator-confirmed. Single reviewer (correctness).

P1 -- High

#8 -- New collections start with an empty admin level, so anyone can administrate them -- src/services/images/dynamic/operations.ts:114 (decision gate -- do not auto-apply)

• Why it matters: createCollection mints two Visibility rows with zero requirements; checkVisibility is vacuously true for an empty list; RequireLevelFromDoubleLevelVisibility is declared USER_NOT_REQUIERED_FOR_AUTHORIZED. So destroyDynamicImageCollectionAction succeeds with no session at all. This is not only a transient window on new collections: seedImages creates the permanent "seeded cms images" collection the same way, and migrateImageCollections gives every OmegaWeb collection visibilityAdmin: { create: {} } under its own //TODO: not everyone should be able to update this.....
• The decision (two options, different scope):
  • (a) Seed visibilityAdmin from the creating session inside createCollection's transaction. Fixes new collections only.
  • (b) Make RequireLevelFromDoubleLevelVisibility treat an empty matrix as unsatisfiable when level === 'ADMIN'. Also closes seeded and migrated collections; IMAGE_ADMIN stays the recovery path. This also removes the ordering constraint the PR body documents ("the admin level must be narrowed before the regular level").
  • (b) is the broader fix; (a) alone leaves seeded and migrated collections open.
• Confidence: 100, validator-confirmed. Corroborated by three independent reviewers (correctness, security, adversarial) -- the strongest agreement in this review.

#9 -- Standard-image lookups fan out into ~30 uncached DB round trips on every page -- src/services/images/standard/operations.ts:106

• Why it matters: src/app/layout.tsx:47 awaits readAllStandardImagesAction() on every request. That loops all 14 StandardImage members; each does a findUnique plus a standardImagesImagePanelOperations.readCollection({}) that re-resolves the same STANDARDIMAGES collection every iteration. NavBar and Footer then render their own StandardImageServer calls on top. No request-scoped cache exists anywhere under src/services.
• Fix (mechanical): wrap the collection lookup (and ideally readStandardImage) in React's cache().
• Confidence: 100, validator-confirmed. Single reviewer (performance).

#10 -- destroyImage unlinks files inside the DB transaction and aborts on the first missing file -- src/services/images/subservice/operations.ts:181

• Why it matters: destroyImage deletes the row, then unlinks four files. Callers (updateProfileImage, committee updateLogo) invoke it with prisma: tx inside prisma.$transaction(..., { timeout: 20000 }). Filesystem writes do not roll back, so a later failure restores the Image row with its files already gone. Separately, implementStore.destroyFile throws NOT FOUND on ENOENT, so one already-missing file aborts the remaining three unlinks.
• Fix (mechanical): split into a DB-only part returning the four fsLocations plus a post-commit cleanup step; make destroyFile swallow ENOENT; use Promise.allSettled for the four unlinks.
• Confidence: 100, validator-confirmed. Corroborated by two independent reviewers (correctness, adversarial).

#11 -- Destroying an image collection deletes the rows but leaves every file in the store -- src/services/images/subservice/operations.ts:33

• Why it matters: The confirmation dialog says "Dette vil ogsa slette alle bilder i salingen", but destroyCollection deletes only the ImageCollection row and relies on onDelete: Cascade for the Image rows. It never calls imageStore.destroyFile, and no caller compensates. Every deletion orphans four files per image with no DB row left to find them by. Store-volume usage grows unbounded. committeeOperations.destroy (committees/operations.ts:214-218) already shows the correct pattern.
• Fix (mechanical): read the collection's fsLocations before the transaction, unlink them after it commits (ignoring ENOENT), and add a test asserting the store has no residue.
• Confidence: 100, validator-confirmed. Corroborated by four independent reviewers (correctness, testing, reliability, adversarial).

#12 -- Collection cover image accepts any imageId without an ownership check -- src/services/images/subservice/operations.ts:57

• Why it matters: updateCollection connects coverImage to a caller-supplied coverImageId with no check that the image belongs to a collection the session administrates. Reading the collection back returns the full Image row (coverImage: true in the includer), whose fsLocation* values map onto the unauthenticated /store/images/<uuid> URLs. Iterating image IDs turns this into readout of restricted collections. The bar is low: IMAGE_COLLECTION_CREATE is a committee permission and a self-created collection makes the caller its administrator. This PR already added the equivalent guard on the sibling path (cmsImageOperations.update refuses an imageId unless sessionAdministratesCollectionOfImage).
• Fix (mechanical): reuse sessionAdministratesCollectionOfImage (extract it from cms/images/operations.ts:23), or require the image to belong to the collection being updated and throw Smorekopp('UNAUTHORIZED', ...) otherwise.
• Confidence: 75, validator-confirmed. Single reviewer (security).

#13 -- Image encoding and file writes run inside the DB transaction, papered over with a 20s timeout -- src/services/users/operations.ts:580

• Why it matters: uploadImage resizes to three sizes, avif-encodes each, and writes four files -- all inside prisma.$transaction. The author's own comment says this is "comfortably slower than the default 5000ms interactive transaction timeout under load", and the response was to raise every affected transaction to 20s. A pool connection and any held locks stay checked out across CPU-bound encoding and filesystem I/O. Same pattern in flairs.updateImage, committees.create/updateLogo, and ombul.updateCoverImage.
• Fix (design-shaped but concrete): two phases -- do the encode/store work outside the transaction, then open a short transaction that only writes the already-produced fsLocation/ext values.
• Confidence: 75, validator-confirmed. Single reviewer (reliability).

P2 -- Moderate

#14 -- Double-level visibility matrix is re-fetched 2-3x per authorized request -- src/services/visibility/implement.ts:113

• Why it matters: readDoubleLevelMatrix reads the matrix in the authorizer, then the operation body discards it and reads it again. updateRegularLevel/updateAdminLevel read it three times (authorizer, ownershipCheck, beforeRun). Every collection detail page pays for this.
• Fix (mechanical): memoize readDoubleLevelMatrixInternal with cache(), or add a fourth AuthorizerFactory type parameter (as RequireVisibilityFilter already has) so the hooks receive the matrix the authorizer already computed.
• Confidence: 100, validator-confirmed. Single reviewer (performance).

#15 -- Creating a collection navigates to a route that does not exist -- src/app/image-collections/MakeNewCollection.tsx:21

• Why it matters: Post-create navigation goes to /image-collections/${collection.id}, but the only routes under src/app/image-collections are dynamic/[name] and special/[specialName]. Creating a collection 404s.
• Fix (mechanical): navigate by name, matching CollectionCardLink.collectionHref: /image-collections/dynamic/${encodeURIComponent(collection.name)}.
• Confidence: 75, validator-confirmed. Single reviewer (adversarial).

Pre-existing (does not count toward the verdict)

• src/prisma/seeder/src/dobbelOmega/migrateImages.ts:118 -- the OmegaWeb image migration is still hard-capped at 10 images by a literal .slice(0, 10), after the limits filter has already trimmed the set. P2. If a debug cap is wanted, drive it from limits.

Coverage

• 10 reviewers dispatched, 10 returned. No failures.
• Cross-model adversarial pass: not run -- the reviewed head is not the working tree (pr-remote), where reviewers must inspect fetched refs. The adversarial lens ran via the in-process adversarial-reviewer fallback, as the routing rule requires. Its agreement with the session-model reviewers is therefore same-family corroboration, not cross-model.
• Validation: one batch, all 15 merged findings validated (no shortcut skips -- no cross-model corroboration existed to license one). 11 confirmed, 4 dropped as validated:false, all from data-migration:
  • Flair/Committee/Ombul FK retarget CmsImage -> Image with no id remap (was P0)
  • visibilityReadId -> visibilityRegularId rename without @map (was P0)
  • Ombul.paragraphId newly required with no backfill (was P0)
  • SpecialCmsImage dropping enum members still referenced by live rows (was P0)
  • Common reason: the harm requires an incremental-DDL path that does not exist in this repo. prisma.config.ts sets migrations.path: '' with a TODO: Add migrations before production; production's Dockerfile/compose run only build+start with no db push or migrate; the sole schema-application script is seed (prisma db push --force-reset). The validator also confirmed migrateOmbul.ts now creates the paragraph via paragraph: { create: {} }, and that no code at head still references the dropped enum members. See the first residual risk below -- the underlying concern is real, it just is not this PR's defect.
• Suppressed by the confidence gate: 3 findings at anchor 50 (concurrent level-update races, concurrent standard-image regeneration, special-collection regeneration leaking Visibility rows) -- all preserved as residual risks below.
• Demoted to soft buckets: 5 single-reviewer P2 advisories (2 testing-coverage, 1 maintainability, 2 reliability/adversarial).
• Quote-the-line gate: 0 findings demoted for a missing first_evidence.
• Untracked files: none excluded. Plan discovery: no plan found under docs/plans/, so settlement suppression was not evaluated.
• No learnings corpus (docs/solutions/ is empty), so learnings-researcher did not run. agent-native and deployment-verification were not selected.

Residual risks

1. No forward-migration mechanism at all. prisma.config.ts has migrations.path: '' with TODO: Add migrations before production; the only schema-apply command is prisma db push --force-reset (full wipe + reseed). That is what invalidated the four schema findings above -- but it also means this PR's destructive DDL (an FK retarget, a required-column addition, an enum narrowing, a column rename) has no defined path to a populated production database. Pre-existing, and the highest-blast-radius item in this PR series. Worth an explicit deploy decision before OWTR #1 ships.
2. Image bytes are not behind the new visibility model. nginx serves /store/images/<uuid>.<ext> with no authorization, so visibility hides listings and metadata only. Anyone who obtains an fsLocation keeps access to a restricted image forever. Pre-existing, but this PR is what makes visibility look like an access-control boundary for image content.
3. standardStoreRoot is derived from import.meta.url (src/lib/standardStore/files.ts:10). Proven for the unbundled seeder scripts; unconfirmed once images/standard/operations.ts pulls it into the Next.js production bundle, where the module sits at a chunk-dependent depth under .next/server/. If the three .. segments miss, every runtime standard-image self-heal throws ENOENT on a public path. Needs a prod build to settle.
4. readStandardImage is RequireNothing but performs writes (delete + re-upload) when a standard image is missing or has escaped the standard collection. In a degraded state that is a repeatable unauthenticated write/disk-consumption path, and the delete bypasses destroyImage, orphaning the old files. Concurrent regeneration can also collide on the standardImage unique index.
5. uploadImage writes four files before prisma.image.create; a failed create strands them (the mirror image of #11, also reachable via uploadManyImages).
6. Concurrent updateRegularLevel + updateAdminLevel each validate the sub-visibility invariant against a stale peer level, so two concurrent saves can leave the pair violating it.
7. implementStore joins an unsanitized dynamicStorePrefix into the path, and destroyFile never normalizes fsLocation. No current caller passes user input, so this is latent, not exploitable today (call-site coverage is grep-only).
8. A failed visibility read collapses to the same null as "not permitted" in image-collections/dynamic/[name]/page.tsx; an admin cannot tell a transient error from a denial, and nothing is logged.
9. dynamicImageOperations.ownershipCheck calls the full readCollection rather than an internal call, adding an undocumented REGULAR-visibility requirement to every dynamic image mutation and surfacing NOT FOUND instead of DISSALLOWED for special collections.
10. Special collections are created with two blank visibilities; they currently fall closed only because readCollection's findFirstOrThrow throws inside ownershipCheck -- an exception, not an authorization decision.
11. expandImageCollection falls back to images[0] with take: 1 and no orderBy, so a collection's implicit cover can change between reads.
12. destroyImage on an image referenced by Ombul.coverImage cascades the Ombul row away (onDelete: Cascade), while Flair.image and Committee.logoImage are Restrict. No reachable exploit constructed, but the asymmetry deserves a deliberate call.
13. The double-level matrix (group ids and omega orders for both levels) is serialized to the client for every session that can read a collection, exposing the composition of restricted groups to regular viewers.
14. uploadAsStandardImage is baked into the generic uploadImage sub-operation, forcing uploadAsStandardImage: null at three unrelated call sites (maintainability, advisory).
15. readDefaultCollectionCover has no fallback if readStandardImage fails, unlike StandardImageServer which degrades gracefully.
16. Seed timeouts were raised 30s -> 60s with maxWorkers: CI ? 2 : undefined, justified by sharp/avif CPU contention. Reasonable, but nothing regression-checks seed duration, so future slowdowns eat the new budget silently.
17. implementDoubleLevelVisibilityOperations has exactly one production consumer today; its generality is unproven until a second domain adopts it.
18. The only formal review on the PR is a Copilot bot review stating it could not review because the PR exceeds 300 files -- so no automated review coverage exists on this PR today. The one substantive human comment ("I suggest someone to user-test this that the visibility authoring makes sense") is unresolved and is a UX-validation ask, not a code change.
19. Verified clean (not a risk): the old VisiblityAdmin typo directory, ImageList, src/app/images/, the duplicated standard_store, and ombul/ConfigVars.ts are all fully deleted at the PR head -- clean renames, no leftovers.

Testing gaps

• No test asserts the store is empty after destroyCollection -- the gap that hid #11.
• No test drives visibilityOperations.update through a failing re-create to assert the matrix is unchanged rather than emptied (#5).
• No test asserts what an empty admin level authorizes; the suite arranges that state with Session.empty() but never pins the resulting exposure (#8).
• No test covers a non-privileged user changing their own profile image end-to-end (#7).
• No test covers updateCollection with a coverImageId from a collection the session cannot read (#12).
• No test covers the updateRegularLevel/updateAdminLevel ownership check with another owner's visibilityId (implement.ts:128-132, :150-154 are unexercised).
• No test covers readStandardImage's two regeneration branches, or implementSpecialCollection's auto-create-from-config path and its cross-collection destroyImage check.
• No test exercises rollback of an image-update transaction to check whether replacement files are cleaned up.
• No test asserts DB query counts for layout.tsx / NavBar / Footer, so #9 and #14 would silently regress after a fix.
• No test covers the dobbelOmega CmsImage -> Image migration paths, or applies the schema diff to an already-populated database (PrismaTestEnvironment always starts from empty).
• The beforeRun framework hook is exercised only indirectly through its one caller; no direct test of its ordering/abort behavior.

Verdict: Not ready

Three P0s block merge, and all three are small, mechanical fixes: #7 (a normal member cannot change their own profile picture), #4 (user/committee/interest-group creation throws UNAUTHORIZED without OMEGA_ORDER_READ), and #5 (a failed visibility save empties the matrix, which means "everyone"). Fix those first -- they are one-line-per-site changes.

Then make the one design call: #8, whether an empty ADMIN visibility level should authorize everyone. Choosing option (b) -- treat an empty matrix as unsatisfiable for ADMIN -- also closes seeded and migrated collections and removes the level-ordering constraint the PR body documents as a known wart.

Everything else is mechanical: the store file leaks (#10, #11), the cover-image ownership check (#12), the per-request cache (#9, #14), the transaction split (#13), and the broken post-create redirect (#15).

Separately, and outside this PR's diff: the repo has no forward-migration mechanism, and this PR is the first to ship genuinely destructive DDL. That is what cleared the four schema findings, but it needs a deploy decision before OWTR #1 ships.